Make Brighter logging instance-scoped and explicit - #4185

Code Review: Instance-scoped logging (deprecate static ApplicationLogging)

Thanks for this — a large but very disciplined change (176 files) that fixes a real, well-described problem: the process-wide ApplicationLogging.LoggerFactory causing cross-talk and ObjectDisposedException between Brighter instances. Execution is consistent and the migration doc is excellent. Feedback below, ordered by importance.

Strengths

• Consistent threading. The ILoggerFactory flows cleanly through the object graph (CommandProcessorBuilder/DispatchBuilder → CommandProcessor/OutboxProducerMediator/Dispatcher → ConsumerFactory → Reactor/Proactor/MessagePump → pipeline/transform builders), with a uniform ?? NullLoggerFactory.Instance fallback at every leaf. No NRE surface.
• Source compatibility preserved. All additions are trailing optional params / additive builder methods, so existing call sites still compile.
• Logger categories preserved. Nice detail keeping OutboxProducerMediator's logger categorized as CommandProcessor (matching the old s_logger), so existing log filters keep working.
• Crash-safe fallback. ApplicationLogging.CreateLogger<T>() tolerating a disposed factory is a sensible safety net for not-yet-migrated code.
• No stragglers. Confirmed ApplicationLogging is gone from all of src/ except its (obsolete) definition.

Main concern — migration doc understates the DI behaviour change

The doc says, for "Using the DI extensions (AddBrighter/AddConsumers)? No change required." That's true for CommandProcessor/Dispatcher/pipeline logging, but producers, consumers, outboxes and inboxes are still constructed by the user (e.g. new RmqProducerRegistryFactory(...).Create(), busConfiguration.Outbox) and handed to the configuration before the container exists. In BuildCommandProcessor/BuildOutBoxProducerMediator the container's ILoggerFactory is not injected back into those already-built objects. So even DI users will silently lose transport/store log output unless they thread the factory into those factories themselves.

This is arguably the most surprising regression for existing users (who today get transport logging "for free" via the static). Suggest the doc explicitly call this out under the DI section — not just under "constructing objects yourself" — since DI users do construct their producer registry / outbox manually in practice.

[Obsolete] + TreatWarningsAsErrors is a hard break for some consumers

src/Directory.Build.props sets TreatWarningsAsErrors=true, and the internal build is clean because nothing references the static anymore. But external consumers who reference ApplicationLogging and build with warnings-as-errors will get a build failure (CS0618) on upgrade, not a soft warning. Combined with the silent-logging behaviour change, this is breaking-change territory — please ensure it lands in a major version bump and is surfaced in release notes, not only in the migration guide.

Internal logging handlers in non-DI scenarios

RequestLoggingHandler/UseInboxHandler now take ILoggerFactory? = null. Under DI they get the factory injected when resolved from the container — good. But users who construct handlers via a hand-rolled IAmAHandlerFactory will now get null logging from these built-in handlers unless they thread the factory through. The migration doc covers transports/stores but not these built-in handlers; worth a sentence.

Test coverage — solid, with two gaps worth closing

• InstanceScopedLoggingTests are well-targeted and clearly motivated. However CommandProcessor_LogsThroughItsOwnContainersFactory_NotAnothers asserts only NotEmpty/Empty on captured entries — it leans on incidental PipelineBuilder log output from publishing with no subscribers. If that internal logging ever changes, the test silently weakens rather than failing meaningfully. Consider asserting on a specific known log line/category to pin the intent.
• The new tests exercise the DI path only. There's no direct test for the builder path (CommandProcessorBuilder.ConfigureLogging(...)/DispatchBuilder.ConfigureLogging(...)) or for OutboxProducerMediator instance-scoping. A small test that the explicit builder method routes logs to the supplied factory would lock in the public API contract.

Minor

• ApplicationLogging.LoggerFactory keeps a public mutable setter that is now a no-op w.r.t. Brighter. A user who sets it expecting log output will be silently disappointed; the XML doc notes this, but consider making the no-op nature even more prominent.
• InMemoryScheduler logs via _logger.LogError("...", state) (line ~301) rather than the source-generated Log.* partials used elsewhere — pre-existing, but inconsistent with the codebase's LoggerMessage convention if you're touching the file anyway.

Verdict

Architecturally the right fix, cleanly implemented, with genuinely good docs. The substantive asks: (1) sharpen the migration doc so DI users understand transport/store logging still needs explicit wiring, (2) treat the obsolete-under-TreatWarningsAsErrors + silent-logging combination as a major-version breaking change in release notes, and (3) add builder-path / outbox instance-scoping tests. None block the core mechanism, which looks correct.

🤖 Generated with Claude Code

Review: Instance-scoped logging (deprecate static ApplicationLogging)

Thanks for tackling this — the underlying bug (a process-wide static ILoggerFactory causing cross-talk between parallel Brighter instances and ObjectDisposedException when one container is disposed) is real and the instance-scoping approach is the right fix. Overall this is a careful, mechanical change and it reads well. A few observations below.

Strengths

• Correct fix for a real problem. Flowing an ILoggerFactory instance through the object graph instead of a shared static eliminates both the last-writer-wins cross-talk and the use-after-dispose.
• Loggers are created eagerly in constructors (_logger = (loggerFactory ?? NullLoggerFactory.Instance).CreateLogger<…>()). This is the key safety property — the leaf holds the ILogger, not the factory, so logging keeps working after the owning container is disposed. Good call.
• Backward-compatible surface for callers — trailing optional loggerFactory/logger params and additive ConfigureLogging(...) builder methods.
• Migration is complete. grep confirms no remaining ApplicationLogging.* references anywhere in src/, so TreatWarningsAsErrors against the [Obsolete] type genuinely guarantees the static is dead.
• Good, targeted tests. The three InstanceScopedLoggingTests cases (cross-talk isolation, sibling-dispose safety, null-factory fallback) map directly to the failure modes, and CapturingLoggerProvider throwing ObjectDisposedException on use-after-dispose is a nice way to make the regression observable rather than silent. The Initializer.TestLoggerFactory refactor is clean.

Issues / things to consider

1. Silent loss of logging for user-constructed stores/transports in DI setups (the main user-facing impact).
The DI extensions flow the container factory into CommandProcessor / OutboxProducerMediator / Dispatcher, but outboxes, inboxes and transports are typically user-constructed and passed into the builder (e.g. new MsSqlOutbox(config)), so they now default to NullLogger unless the user explicitly threads a logger/factory in. Previously these logged because the DI extension populated the static. This is acknowledged in the [Obsolete] message and PR body, but it's an easy-to-miss regression for existing DI users relying on outbox/inbox/transport logs. Worth giving prominence in the release notes (you flagged this), and a follow-up to have the relevant DI registrations inject the resolved factory into the stores they build would shrink the silent gap.

2. Framework handler subclasses don't forward the factory to their base RequestHandler<T>.
RequestLoggingHandler, UseInboxHandler, DeferMessageOnErrorHandler, RejectMessageOnErrorHandler each set their own _logger but call the base primary constructor with defaults, e.g.:

public DeferMessageOnErrorHandler(ILoggerFactory? loggerFactory = null)
{
    _logger = (loggerFactory ?? NullLoggerFactory.Instance).CreateLogger<DeferMessageOnErrorHandler<TRequest>>();
}

The base RequestHandler<TRequest> (which logs PassingRequestFromTo / FallingBackFromTo) therefore stays null-backed even when a factory is supplied. Minor — only the base-class trace lines are affected — but forwarding (: base(loggerFactory: loggerFactory)) would make it consistent.

3. OutboxProducerMediator logs under the wrong category (pre-existing).
_logger = loggerFactory.CreateLogger<CommandProcessor>(); — the mediator creates its logger with the CommandProcessor category rather than its own type. This is carried over verbatim from the old ApplicationLogging.CreateLogger<CommandProcessor>(), so it's not introduced here, but since every line in this ctor is being touched it'd be a low-risk moment to correct it to the mediator's own type. (Respecting the repo's "Change Scope" guidance, leaving it is also defensible — your call.)

4. Adding ConfigureLogging to public builder interfaces is technically binary-breaking.
IAmACommandProcessorBuilder.ConfigureLogging and IAmADispatchBuilder.ConfigureLogging are new members on public interfaces. Source-compat is preserved for callers, but any external implementer of these interfaces would break. Almost certainly fine given these are framework builder contracts unlikely to be user-implemented — just noting it for the changelog/semver framing.

Performance & security

No concerns. The change is allocation-neutral in the hot path (loggers created once at construction; NullLoggerFactory.Instance/NullLogger<T>.Instance are singletons), and there are no security-relevant surfaces.

Test coverage

Solid for the regression itself. Two optional additions worth considering: (a) a test asserting the CommandProcessorBuilder.ConfigureLogging(...) non-DI path actually routes through the supplied factory, and (b) a case covering the "user-constructed outbox without a factory logs to no-op" behavior change, to pin the documented contract.

Nice work overall — the core mechanism is sound and the eager-creation detail is exactly right.

Reviewed by Claude — automated PR review.

Review: Make Brighter logging instance-scoped (#4185)

Thanks for tackling this — routing logging through a process-wide static was a genuine latent bug, and the writeup of the two failure modes (last-writer-wins cross-talk + use-after-dispose on container disposal) is spot on. The overall approach is sound and the execution is consistent across a large surface (175 files). A few notes below, mostly minor.

What works well

• Right fix, right shape. Flowing an ILoggerFactory as an instance through the object graph is the correct model, and keeping every change additive (trailing optional ILoggerFactory? loggerFactory = null / ILogger? logger = null, additive ConfigureLogging builder methods) preserves source compatibility for existing call sites.
• Consistent null-fallback. The ?? NullLoggerFactory.Instance / ?? NullLogger<T>.Instance pattern is applied uniformly, so a missing factory degrades to a no-op rather than throwing. The NoLoggerFactoryRegistered_FallsBackSafely test pins this.
• Sensible DI division of labour. Types the host/ActivatorUtilities construct (e.g. TimedOutboxSweeper, the relational stores) take ILogger<T> so DI auto-injects, while manually-constructed types (OutboxArchiver, CommandProcessor, mediator) get the factory passed explicitly. That's the correct split.
• Obsolete shim is well done. ApplicationLogging defaulting to NullLoggerFactory.Instance and CreateLogger<T>() swallowing ObjectDisposedException makes the deprecated path crash-free, and the [Obsolete] message points users to all three migration routes.
• Tests target the actual bugs. InstanceScopedLoggingTests exercises exactly the cross-talk and dispose-a-sibling scenarios that motivated the change, and CapturingLoggerProvider throwing ObjectDisposedException after dispose is a nice way to surface a regression if a factory reference ever leaks again.

Observations / questions

1. Message-creator leaves silently lost their logging path (main substantive item). KafkaMessageCreator, RedisMessageCreator, and both RmqMessageCreators still hold a hardcoded static logger, now NullLoggerFactory.Instance.CreateLogger<...>() instead of ApplicationLogging.CreateLogger<...>(). Previously, in a DI setup, these routed through the container's factory; now their diagnostics (malformed-header warnings, correlation-id/handled-count/delay parse failures, FailedToReadTheValueOfHeader, etc.) are dropped even when the user has configured logging. The owning consumer already has a loggerFactory in scope (e.g. RmqMessageConsumer), so this looks threadable. Suggest either threading the factory down (making the creator instance-scoped where it's static-called) or explicitly calling out in the PR description that these creator diagnostics become no-ops — right now this is a quiet observability regression that the "user-constructed transports/stores" caveat doesn't quite cover, since these are Brighter-constructed.

2. Behaviour change is runtime-silent, not just compile-time. Code reading ApplicationLogging.LoggerFactory (yours or third-party) still compiles but now silently observes a no-op factory rather than the container's. The [Obsolete] attribute only nudges at compile time. Please make sure the release notes call this out prominently as a behavioural break, not just a deprecation — the PR body flags it as "worth a note," which I'd upgrade to "must."

3. Test coverage skews to the CommandProcessor/DI path. The new instance-scoping tests only cover IAmACommandProcessor. The Dispatcher/MessagePump/Reactor/Proactor threading is exercised only indirectly via the four updated MessageDispatch tests. A small test asserting two Dispatchers don't cross-talk would lock in the consumer side too, given that's half the object graph touched here. Optional given the scope.

4. Minor consistency nit. Relational stores take ILogger? while everything else takes ILoggerFactory?. The rationale (DI injects ILogger<T> directly for these) is sound, but the asymmetry is non-obvious — a one-line comment at those constructors explaining "factory is injected as ILogger<T> by DI here" would help future maintainers.

5. InstanceScopedLoggingTests assertions lean on an implementation detail. Both passing tests assert the other container's capture is Empty, which holds only because resolving an IAmACommandProcessor doesn't itself log. That's true today and the tests are fine, but if construction ever starts logging these become confusingly red. A comment noting "resolution must not log" would document the assumption.

Verification

• Confirmed no remaining ApplicationLogging or live s_logger references in src/ outside the obsolete class and the four message creators (which are intentionally retained as no-ops).
• [Obsolete] usage inside the DI extension is correctly pragma-suppressed; with TreatWarningsAsErrors a clean build implies no stray references to the obsolete static — consistent with the PR's claim.

Nice, careful piece of work overall. The message-creator gap (#1) is the only thing I'd want resolved or explicitly documented before merge; the rest are polish.

🤖 Generated with Claude Code

Alright, we will need to look over this one carefully, as it was previously a choice to avoid passing loggers throughout the application. It may not be a breaking change, but it may change behavior for V10 users who don't transition, potentially resulting in a silent failure. That would mean, IMO, it would be V11, and it would be preferable to force success rather than allow silent failure. In addition, it may change log messages.

Not necessarily adverse, but didn't really want to trigger V11 work until SEP/OCT

Agreed it's a breaking change for anyone manually constructing these classes. We can hold off until V11 if you were happy with this change and park for now if needed.

For MEDI users I don't believe they'd have to tweak any code.

But also happy to discuss the design further. The current behaviour is a problem though for any two or more brighter's running under the same process. We noticed it earlier in a test suite.

What do you mean by force success btw? Remove the NullLogger fallback?

Review: Make Brighter logging instance-scoped (#4185)

Thanks for tackling this — the use-after-dispose and cross-talk caused by the process-wide static were real, nasty bugs (especially under parallel test suites), and the instance-scoped approach is the right fix. The refactor is large but overwhelmingly mechanical and consistent: s_logger → _logger, an optional trailing ILoggerFactory? loggerFactory = null threaded through the object graph, and a uniform ?? NullLoggerFactory.Instance fallback that keeps everything crash-free. I traced the DI → builder → CommandProcessor/Dispatcher → ConsumerFactory → Reactor/Proactor/MessagePump → PipelineBuilder/TransformPipelineBuilder paths and the wiring is correct end-to-end. [Obsolete]-ing ApplicationLogging rather than deleting it, plus the TreatWarningsAsErrors gate to prove no internal references remain, is a clean migration.

A few things worth addressing or at least calling out:

1. Message creators silently lose logging entirely (behavioural regression, not in the PR description)

RmqMessageCreator (sync + async), KafkaMessageCreator, and RedisMessageCreator were changed from ApplicationLogging.CreateLogger<T>() to a hardwired NullLogger<T>.Instance:

private static readonly ILogger s_logger = NullLogger<RmqMessageCreator>.Instance;

Under DI, the old static was populated with the container's real factory, so these creators previously logged through it. Now they log to a no-op for everyone, with no way to ever supply a real logger. The messages being silenced are exactly the diagnostics an operator wants — ExpectedHeaderError, ExpectedBodyError, FailedToCreateMessageFromKafkaOffset, etc. — i.e. "we received a malformed/unparseable message."

• KafkaMessageCreator.CreateMessage is an instance method, and KafkaMessageConsumer does _creator = new KafkaMessageCreator(); at KafkaMessageConsumer.cs:270 while already holding a loggerFactory. Threading the factory through here is trivial and would preserve the diagnostics — this one looks like a straightforward miss.
• RmqMessageCreator / RedisMessageCreator use static CreateMessage methods, so it's more awkward — but an ILogger parameter on the static method (passed from the calling consumer) would keep them alive.

If silencing these is intentional, it should at least be noted in the PR's "Behaviour change" section; right now that section only mentions user-constructed transports/stores falling back to no-op, not these always-on-no-op leaf objects.

2. Interface addition is a breaking change for implementers (minor)

ConfigureLogging(ILoggerFactory) is added to the public IAmACommandProcessorBuilder interface (and the DispatchBuilder equivalent). The PR says "all public constructor/builder changes are trailing optional parameters / additive methods, so existing call sites remain source-compatible" — true for callers, but adding a member to a public interface is a source/binary break for anyone who implements it. Unlikely anyone implements Brighter's builder interfaces, but the claim is slightly overstated; worth a one-line caveat in the release notes.

3. Per-message CreateLogger on the hot path (minor perf)

s_logger was previously created once per type. Now PipelineBuilder<T> is constructed per Send/Publish and calls loggerFactory.CreateLogger<PipelineBuilder<T>>() each time, and RequestHandler<T>'s primary constructor calls CreateLogger per handler instantiation (i.e. per message). The default LoggerFactory.CreateLogger caches by category but takes a lock to do so, so this adds a small amount of locked dictionary work per message/per handler on the dispatch hot path. Almost certainly negligible, but flagging since the previous static avoided it entirely. Not a blocker.

Test coverage

InstanceScopedLoggingTests is well-targeted — it directly exercises the three behaviours that motivated the change (no cross-talk, survives sibling-container disposal, safe fallback with no factory registered), and the CapturingLoggerProvider throwing ObjectDisposedException post-dispose is a nice way to actually surface a use-after-dispose rather than just asserting on output. The Initializer.TestLoggerFactory migration for the four MessageDispatch tests is clean.

Gap: the new tests only cover the CommandProcessor/DI path. There's no coverage for the Dispatcher/ServiceActivator instance-scoped path or for transport leaf objects receiving the factory — which is relevant to finding #1 above.

Summary

Solid, well-executed fix for a genuine concurrency/disposal bug. The main thing I'd want addressed before merge is #1 — at minimum thread the factory into KafkaMessageCreator, and document the message-creator logging loss in the behaviour-change notes. The rest are minor / release-note items.

Review: Instance-scoped logging (deprecate static ApplicationLogging)

Thanks for tackling this — moving off the process-wide static is the right call, and issue #4184 is a real correctness bug (cross-talk + use-after-dispose between Brighter instances). The approach is clean: trailing optional ILoggerFactory?/ILogger? parameters and additive ConfigureLogging(...) builder methods keep every public call site source-compatible, and the DI extensions now flow the container factory through as an instance instead of writing the static. I verified there are no remaining references to ApplicationLogging or s_logger anywhere in src/, so the migration is complete and consistent.

A few observations, mostly minor, plus one I would like you to confirm is intentional.

1. Base RequestHandler relay logs are now silent in all paths (please confirm intent)

RequestHandler<TRequest> / RequestHandlerAsync<TRequest> gained a primary-constructor ILoggerFactory? loggerFactory = null, and _logger falls back to NullLoggerFactory.Instance when it is null (src/Paramore.Brighter/RequestHandler.cs:52, RequestHandlerAsync.cs). The problem: nothing forwards a factory to that base constructor. I grepped — no built-in handler calls : base(loggerFactory: ...), and the MS DI container cannot satisfy a base-class primary-constructor parameter (it binds the derived constructor). The built-in handlers that take a logger (RequestLoggingHandler, UseInboxHandler, etc.) inject their own ILogger<T> for their own log lines, but when they call base.Handle(...) the base-class relay lines — Log.PassingRequestFromTo / Log.FallingBackFromTo (RequestHandler.cs:113,148) and the async equivalents — go to NullLogger. Pre-change, those lines logged through the DI-populated static, so in the DI happy path they were visible. After this PR they are silent everywhere, including DI. That is an observability regression not covered by the user-constructed transports/stores note in the PR description. If it is a deliberate trade-off (these are verbose debug lines) it would be worth saying so in the migration notes; otherwise the base needs a way to receive the factory (e.g. PipelineBuilder setting it on handlers it chains, since PipelineBuilder already holds a logger).

2. OutboxProducerMediator logger category

_logger = loggerFactory.CreateLogger<CommandProcessor>() (src/Paramore.Brighter/OutboxProducerMediator.cs:121) — categorised as CommandProcessor, not OutboxProducerMediator. This faithfully preserves the old static category, so it is not a regression, but since you are already touching every line it would be a good moment to fix the mislabelled category (or leave a comment that it is deliberate for log-filter back-compat).

3. Activator.CreateInstance positional binding (not a bug, just fragile)

BuildOutBoxProducerMediator passes loggerFactory as the trailing positional arg to Activator.CreateInstance (ServiceCollectionExtensions.cs:754). This is correct today because loggerFactory is the last constructor parameter — but it is reflection-based, so an arg-order mismatch would compile fine and throw only at runtime. Worth a one-line comment tying the positional order to the constructor signature so a future reorder does not silently break it.

Test coverage

The new InstanceScopedLoggingTests + CapturingLoggerProvider are nicely targeted — the dispose-A-does-not-break-B and no-factory-registered-falls-back-safely cases directly pin the #4184 bug, and the capturing provider throwing ObjectDisposedException on use-after-dispose is a clean way to surface the original failure mode. Gaps worth closing: (a) all three tests publish an event with no subscribers, so they only exercise the PipelineBuilder log lines via CommandProcessor._logger — they never run an actual handler pipeline, so they do not prove handlers resolved via DI log through the right factory, and would not catch the base-handler regression in point 1; consider a test that registers a real handler and asserts its log output lands in the right capture. (b) No test covers the ConfigureLogging(...) builder path directly (only the DI path) — a quick CommandProcessorBuilder.ConfigureLogging(factory) assertion would lock in the non-DI flow.

Minor

ApplicationLogging.CreateLogger<T>() swallowing ObjectDisposedException to return NullLogger<T> (ApplicationLogging.cs:41) is a sensible crash-free fallback for stragglers; good defensive touch given the type is obsolete-but-present.

Overall this is a solid, well-scoped refactor that fixes a genuine bug with good backward-compat discipline. The main thing I would want resolved before merge is confirming whether the base-handler relay logging going silent (point 1) is intended, and ideally a test that exercises a real handler pipeline.

Automated review — verified against the merge of this branch into master; I was unable to run the build/tests in this environment, so the compile-clean claim is taken on trust.

What do you mean by force success btw? Remove the NullLogger fallback?

Yes, because I get that stops users having compilation issues, but it feels like it could silently fail on folks who where not expecting it.

But also happy to discuss the design further. The current behaviour is a problem though for any two or more brighter's running under the same process. We noticed it earlier in a test suite.

I think it may help with some other aspects like testing too. We currently have to use a test capture context and I am not sure that is the best route

Review: Make Brighter logging instance-scoped and explicit

Thanks for tackling this — the underlying bug is real and nasty (a process-wide static ILoggerFactory where the last container to initialise wins, then disposes it out from under everyone else). Threading the owning container's factory through the object graph is the right shape of fix, and InstanceScopedLoggingTests captures the two failure modes (cross-talk and use-after-dispose) precisely.

Caveat: at 1,434 files this PR exceeds GitHub's diff API limit, so I reviewed the merged tree rather than a line-by-line diff. Where something may predate this PR, I've said so.

1. The samples now silence the logging this PR exists to protect

122 sites across samples/ were patched to NullLoggerFactory.Instance, including the flagship WebAPI sample:

// samples/WebAPI/WebAPI_Common/TransportMaker/ConfigureTransport.cs:83
loggerFactory: global::Microsoft.Extensions.Logging.Abstractions.NullLoggerFactory.Instance)

The PR's stated goal is "missing logger configuration now throws instead of silently disabling logs" — but the samples are the primary documentation for how to wire Brighter, and they now demonstrate exactly the silent-no-op outcome the change set out to eliminate. These are host-builder apps with a real ILoggerFactory available (builder.Services / host.Services.GetRequiredService<ILoggerFactory>()); a user copying ConfigureTransport.cs gets a transport with no diagnostics and no indication why.

Please thread the host's factory through the samples. Where one genuinely can't (a static helper with no host in scope), a one-line comment explaining the deliberate opt-out would help.

Minor, but 1,597 occurrences repo-wide: the fully-qualified global::Microsoft.Extensions.Logging.Abstractions.NullLoggerFactory.Instance reads as mechanical patching. A using Microsoft.Extensions.Logging.Abstractions; (or a shared test constant for the 1,475 test-side uses) would be considerably more idiomatic.

2. Logger category mismatches

The sweep makes these visible; several loggers are created under the wrong category, which breaks per-category filtering for anyone configuring Logging:LogLevel by namespace:

Some likely carried over verbatim from the old ApplicationLogging.CreateLogger<T>() calls, but since every one of these lines is being touched anyway, this is the cheap moment to fix them.

3. No public escape hatch for a directly constructed handler

RequestHandler<T>.Logger (src/Paramore.Brighter/RequestHandler.cs:55) throws InvalidOperationException when unconfigured, and IRequireLoggerFactory is internal (line 181) with no InternalsVisibleTo. PipelineBuilder covers every in-framework construction path (I checked all six call sites — top handler plus all four decorator paths), so the runtime path is sound.

The problem is outside the framework. Constructing a handler chain by hand is a very common user unit-testing pattern:

var handler = new MyHandler();
handler.SetSuccessor(new MyOtherHandler());
handler.Handle(command);   // base.Handle -> Log.PassingRequestFromTo(Logger, ...) -> throws

That now throws, and because the interface is internal there is no public API a user can call to fix it. Turning a logging concern into a hard failure in a branch users can't reach is a rough edge. Options, roughly in order of preference: make IRequireLoggerFactory public; add a protected ConfigureLogging/settable logger on the base class; or let the un-configured direct-construction path fall back to NullLogger while keeping fail-fast on the builder paths, where a missing factory really is a configuration error.

4. The two new fail-fast paths are untested

The PR body highlights "Require ConfigureLogging(ILoggerFactory) before CommandProcessorBuilder.Build() and DispatchBuilder.Build()", but nothing asserts it — I found no test reaching either "A logger factory is required. Call ConfigureLogging before Build." throw (CommandProcessorBuilder.cs:295, DispatchBuilder.cs:186). Only the DI path is covered, by When_no_logger_factory_is_registered_should_fail_fast. Two small tests would lock in the headline behaviour change; per CLAUDE.md's TDD workflow they should have come first via /test-first.

Also worth considering: make logging a stage in the progressive builder interface (INeedALoggerFactory) so the compiler enforces it rather than Build(). That's how the builder already handles handlers, resilience, and the context factory.

5. Error-message quality on the DI path

GetRequiredService<ILoggerFactory>() produces the stock "No service for type 'Microsoft.Extensions.Logging.ILoggerFactory' has been registered" — the assertion on bare InvalidOperationException at InstanceScopedLoggingTests.cs:108 confirms there's no Brighter-branded guidance. Given this is a breaking behaviour change people will hit on upgrade, wrapping it in a ConfigurationException that names the two fixes (AddLogging(...), or register NullLoggerFactory.Instance) would save a lot of issue traffic.

6. XML documentation

190 new ILoggerFactory loggerFactory parameters, 81 <param name="loggerFactory"> entries. CommandProcessor.cs, Dispatcher.cs, MessagePump.cs and PipelineBuilder.cs have none, and the existing <param> blocks on the CommandProcessor constructors are now out of order relative to the signatures. AzureBlobLockingProvider.cs:37 has an empty <param name="loggerFactory"></param>. .agent_instructions/documentation.md requires these on public API.

7. Migration notes understate the break

It isn't only "a new required parameter" — parameter order changed on public constructors. Dispatcher now takes loggerFactory as the 3rd positional argument, ahead of the previously-optional messageMapperRegistry (Dispatcher.cs:137-150); CommandProcessor's constructors similarly moved requestSchedulerFactory up. Positional callers get a compile error rather than silent misbinding (good), but the release notes should say "signature reordered", not "parameter added".

8. No ADR

CLAUDE.md mandates /adr for architectural decisions, and ADRs 0066–0069 document comparable DI/lifetime decisions. This one — removing a process-wide static, mandating explicit ILoggerFactory, and choosing throw over NullLogger — is at least as consequential, and has a genuine trade-off worth recording for the next person who asks "why can't I just get a NullLogger default?".

9. Hot-path CreateLogger calls (follow-up, not blocking)

ILoggerFactory.CreateLogger takes a lock on every call, so these are worth revisiting now that a factory is available as a field on the long-lived owner:

• CommandProcessor.cs:328/405/483/586 — new PipelineBuilder<T>(...) per Send/Publish, whose constructor calls CreateLogger<PipelineBuilder<TRequest>>() (PipelineBuilder.cs:106).
• HandlerLifetimeScope.cs:53 and TransformLifetimeScope.cs:16 — constructed per message.
• ServiceProviderLifetimeScope.cs:83 — per handler pipeline, and this one adds a GetRequiredService<ILoggerFactory>() resolution per message on top. Its _logger is used only in two catch blocks during Dispose (lines 487, 501), so eagerly resolving and creating it for every message is pure overhead; passing a pre-created ILogger in from the owning factory would remove it.

Most of these were presumably equally costly under ApplicationLogging, so I'm not claiming a regression — but the refactor makes the fix trivial.

10. Test-quality nits

• CommandProcessor_LogsThroughItsOwnContainersFactory_NotAnothers depends on Publish with no subscribers emitting at least one Debug line. That's incidental behaviour; a log-level change elsewhere breaks the test for an unrelated reason. Asserting on a known category or message would be sturdier.
• Assert.Empty(captureA.Entries) after providerA.Dispose() is fragile in the same way — it silently depends on CommandProcessor construction emitting nothing.
• CapturingLoggerProvider.IsDisposed is a non-volatile bool read from the logger without the gate held. Fine in practice here, but Entries is carefully locked, so the asymmetry stands out.

Security: nothing of concern. No secrets, no new external surface; the failure mode is a startup exception rather than anything exploitable.

Bottom line: the core change is correct and well-motivated, and the DI wiring is applied consistently — GetRequiredService throughout, no residual NullLogger fallbacks in src/, no static loggers left. I'd want #1 addressed before merge (shipping samples that silently discard logs undercuts the point of the change), plus the category fixes in #2 and the two missing builder tests in #4. The rest can follow up.

Review: Make Brighter logging instance-scoped and explicit

Killing the mutable process-wide ApplicationLogging.LoggerFactory is the right call, and the sweep is impressively complete — zero remaining s_logger/static readonly ILogger in src/, and no NullLoggerFactory fallbacks leaked into production code. InstanceScopedLoggingTests is a genuinely good test: the "resolve B first, then A, then dispose A" setup nails the exact failure mode of the old static.

Below are the things I'd want addressed before merge, roughly by priority. (Comment 1 of 2 — correctness/perf here, scope/process in the follow-up.)

1. Handlers throw InvalidOperationException from a logging statement

RequestHandler.cs:53-59 (and RequestHandlerAsync.cs:54-61):

private ILogger Logger => _logger ?? throw new InvalidOperationException(
    "Handler logging must be configured by PipelineBuilder before the handler executes.");

SetSuccessor is public API, and hand-composing a chain is common in downstream unit tests:

var handler = new MyHandler();
handler.SetSuccessor(new MyOtherHandler());
handler.Handle(cmd);          // now throws InvalidOperationException

Three reasons this is a poor failure mode:

• Latent and conditional — Logger is only touched when _successor != null (RequestHandler.cs:118, :153). The same handler works as a chain leaf and blows up once chained. No compile-time signal.
• It turns a diagnostics concern into a control-flow failure. Nothing about the handler's work needs a logger.
• The goal is removing a mutable static, not forcing per-handler configuration. private ILogger _logger = NullLogger.Instance; is still instance-scoped, still overwritten by PipelineBuilder.ConfigureLogging, and has no trap.

If you'd rather keep fail-fast, please add a test pinning it and call it out in the migration notes — currently neither exists.

2. Per-message CreateLogger on the hot path

LoggerFactory.CreateLogger(string) takes a lock on the factory's _sync for every call, even on a cache hit. The old static readonly s_logger paid that once per closed generic type, per process. These now run per message:

A pipeline with a few attribute decorators now takes ~8-10 factory locks per message where it took zero. Under Publish fan-out and multiple pumps sharing one root factory that's a shared contention point on the throughput path.

Could you run the BenchmarkDotNet suite before/after? If it moves, two cheap fixes: have PipelineBuilder create the handler ILogger once in its ctor and change IRequireLoggerFactory.ConfigureLogging to take the ILogger; and memoise per-type loggers against the factory (e.g. ConditionalWeakTable<ILoggerFactory, ...>), keeping instance scoping while restoring the old amortisation.

3. DispatchBuilder XML doc contradicts the implementation

DispatchBuilder.cs:293 says "If not called, a no-op logger factory is used." — but Build() at :186 throws ConfigurationException. The doc describes the behaviour you deliberately removed. CommandProcessorBuilder.cs:501-503 gets it right.

4. Enforce logging at compile time, not runtime

Both builders use Brighter's progressive-interface idiom (INeedAHandlers → INeedResilience → INeedMessaging → INeedInstrumentation → INeedARequestSchedulerFactory → IAmACommandProcessorBuilder) precisely so required steps are compiler-enforced. Putting ConfigureLogging on the terminal interface makes a newly-mandatory step invisible to the compiler and defers the error to first boot — the one thing that design exists to avoid. An INeedLogging stage (or StartNew(loggerFactory)) turns "you forgot logging" into a build error, which is a far better upgrade experience.

5. Missing null guards on the new ILoggerFactory parameters

Only CommandProcessorBuilder.cs:285 and DispatchBuilder.cs:142 guard. CommandProcessor.cs:167, Dispatcher.cs:154, OutboxProducerMediator.cs:146 and every gateway consumer/producer do a bare loggerFactory.CreateLogger<T>(). For callers without NRTs (or from F#/VB) that's an opaque NullReferenceException inside a constructor rather than ArgumentNullException(nameof(loggerFactory)). Given fail-fast is the thesis, worth being consistent at least at the public entry points.

6. Three injection styles for the same dependency

New public API is hard to change later, so worth settling now:

• ILoggerFactory — the majority
• plain ILogger — TransformerFactory.cs:31, TransformerFactoryAsync, RmqMessageCreator, KafkaMessageCreator, PostgreSqlBoxDetectionHelper
• ILogger<T> — MsSql/ChannelFactory.cs:33, MsSqlOutbox.cs:51, PostgreSqlOutbox, MySqlOutbox, SqliteOutbox

The plain-ILogger form also miscategorises: TransformPipelineBuilder.cs:198 passes its own _logger into TransformerFactory, so FailedToReleaseTransformerAfterInitFailure logs under category TransformPipelineBuilder. Passing _loggerFactory fixes it.

7. Logger-category policy is inconsistent

I diffed every CreateLogger<T> old vs new — categories are faithfully preserved almost everywhere, including odd pre-existing ones (KafkaMessagingGateway → KafkaMessageProducer, AzureBlobLockingProvider → AzureBlobLockingProviderOptions). But TransformPipelineBuilderAsync.cs:87 was silently corrected to TransformPipelineBuilderAsync, while OutboxProducerMediator.cs:146 kept CreateLogger<CommandProcessor>(). Fix all or none — and either way category renames break consumers' appsettings.json log-level filters, so they need a release note.

8. protected fields named _logger break the project's style guide

.agent_instructions/code_style.md:5 — "Use PascalCase for public and protected members". Five violations on public abstract bases (so they are API): MessagePump.cs:75, Mediator/Steps.cs:61, KafkaMessagingGateway.cs:43-44, AWSSQS/AWSMessagingGateway.cs:48, AWSSQS.V4/AWSMessagingGateway.cs:48. RmqMessageGateway.cs:61 (LoggerFactory) and SqlBoxMigrationRunner.cs:65 (Logger) get it right.

(Comment 2 of 2 — scope, samples, tests, process.)

9. Scope creep

CLAUDE.md: "Do NOT change defaults or make changes beyond what was explicitly requested... no additional 'improvements' or default value changes."

git diff --ignore-all-space reports 6,471/5,266 against the PR's 9,458/8,253 — roughly 3,000 lines of pure whitespace churn, plus BOM additions (CommandProcessorBuilder.cs:1, ServiceProviderLifetimeScope.cs:1). Unrelated behavioural/API edits are also mixed in:

• TransformPipelineBuilderAsync.cs:78 — InstrumentationOptions gained a default = InstrumentationOptions.All. That's a default change, explicitly out of bounds per CLAUDE.md.
• ServiceCollectionExtensions.cs:248 — AddProducers's configure widened to Action<ProducersConfiguration>?.
• RelationDatabaseOutbox.cs — ~20 public signatures widened RequestContext → RequestContext?. Safe (bodies already used requestContext?.Span), but it's a public nullability contract change.
• MessagePump.cs:69-73, 158-168 — the protected ctor replaced by a MessagePumpConfiguration record struct. A parameter-object refactor that breaks every third-party pump, independent of logging.
• KafkaMessageConsumer.cs:265-288, OutboxProducerMediator.cs:186-210 — extract-method refactors (CreateRejectionProducers, SweepOffsetsIfOpen, ConfigureOutbox, ValidateMessagingDependencies).

The formatter also produced this in ~10 places, which is worse than what it replaced:

try
{ CleanUpAfterFailedBuild(pipeline, transformLeases, messageMapperLease); }
catch (Exception cleanupException) { Log.FailedToCleanUpAfterFailedBuild(_logger, cleanupException); }

Per /tidy-first, could the formatting and extract-method work land as a separate structural commit (ideally its own PR)? On a breaking change, ~3k lines of noise actively hides the behavioural diff from reviewers.

10. Samples now teach users to disable logging, in unreadable code

samples/WebAPI/WebAPI_Common/DbMaker/OutboxFactory.cs:77:

return (new PostgreSqlOutbox(configuration, logger: global::Microsoft.Extensions.Logging.LoggerFactoryExtensions.CreateLogger<global::Paramore.Brighter.Outbox.PostgreSql.PostgreSqlOutbox>(global::Microsoft.Extensions.Logging.Abstractions.NullLoggerFactory.Instance)), ...);

This is machine-generated output (fully-qualified global:: everywhere, LoggerFactoryExtensions.CreateLogger<T>(factory) static-call form instead of factory.CreateLogger<T>()). 672 files across samples/ and tests/ now contain global::Microsoft.Extensions.Logging..., with 106 NullLoggerFactory.Instance wire-ups in samples/ alone. src/ is clean, so the fixer clearly wasn't run there — please re-run it over tests and samples with using directives.

The semantic problem is bigger than the formatting one: samples are the primary documentation, and every one now demonstrates how to turn Brighter's logging off. Someone copy-pasting samples/TaskQueue/RMQTaskQueue/GreetingsSender/Program.cs gets a silently log-free Brighter. Samples should show the recommended path — a real ILoggerFactory from AddLogging(...) / the host — with NullLoggerFactory reserved for tests where suppression is the point.

11. Test coverage gaps on the headline behaviour change

InstanceScopedLoggingTests covers the DI paths well. Not covered:

• CommandProcessorBuilder.Build() throwing ConfigurationException when ConfigureLogging wasn't called. grep -rln "A logger factory is required" tests/ returns nothing — and this is the documented behaviour change of the PR.
• Same for DispatchBuilder.Build().
• The RequestHandler/RequestHandlerAsync unconfigured-Logger path from §1.
• A regression test that every handler and decorator in a built pipeline receives ConfigureLogging. There are six easy-to-miss call sites (PipelineBuilder.cs:204, 250, 452, 476, 524, 552); a future decorator path that forgets one fails only at runtime, only when chained.

Per CLAUDE.md's TDD workflow these should have been driven by /test-first.

12. ADRs and migration docs

Two accepted ADRs were retro-edited rather than superseded: 0057 (rewriting the "Constructor injection (additive) … non-breaking" paragraph) and 0064 (deleting the OOS-2 paragraph that deliberately left the async builder's logger generic alone — which §7 then changes). Editing accepted decisions in place erases the record of why the earlier call was made and that it was later reversed. A new ADR for instance-scoped logging that supersedes those points, plus amendment notes on 0057/0064, is the conventional shape.

There's also no migration/upgrade doc for a change that breaks nearly every public constructor in the library. The PR body's "Behaviour change" section is a good start; it should live in docs/ (and release notes) with before/after for the common paths: AddBrighter() with no AddLogging(), hand-rolled CommandProcessorBuilder, hand-constructed outboxes/consumers.

Security

Nothing concerning. No new secret or PII surface — this moves logger plumbing, not payloads. If anything, removing a globally-settable ApplicationLogging.LoggerFactory closes a small hole: any code in the process could previously redirect all of Brighter's logs by assigning that static.

Summary

Good idea, thorough sweep, and the right tests for the DI paths. My blockers are §1 (handlers throwing from a log statement) and §3 (doc contradicting behaviour); §2 wants a benchmark before merge; §9 and §10 are the ones I'd most like split out, since ~3k lines of formatting noise plus 672 machine-mangled test/sample files make a breaking change materially harder to both review and adopt.